fix: Handle Unnest LogicalPlan in FederationOptimizer with_new_exprs #135
Conversation
Fix critical issue where Unnest LogicalPlan.expressions() returns expressions but with_new_exprs() rejects them, causing federation optimizer to panic. - Add special case in optimize_plan_recursively to pass empty expressions for LogicalPlan::Unnest instead of plan.expressions() - Add comprehensive test demonstrating federated unnest across multiple engines - Add regression test for FederationOptimizerRule.rewrite handling Unnest plans This enables proper federation of queries containing unnest operations.
|
Hello @GeorgeLeePatterson, Thanks for this PR I think we should consider holding off on merging this for now and instead wait for a proper fix upstream in DataFusion itself. Not a big fan of adding special-case handling directly into the optimizer, it can make the logic harder to read & reason about over time Let's wait the upstream fix and revisit this once it's available @trueleo Maybe we can get your thoughts on this as well? |
|
@hozan23 Thank you for the reply. That makes sense, I can continue operating on my fork for now in the context of an upcoming crate I am releasing. I have code locally with the fix for DataFusion. If your goal is to benefit from the upstream fix first, then I can shift my focus in that direction and see if I can get the fix merged on the DataFusion side. I appreciate the response and let me know if there's anything else here that is of use. If you do decide to merge it, I will be posting a PR for the DataFusion fix regardless and would be happy to track it to ensure this library gets updated as well. |
2 participants
Summary
Fixes a critical issue where the federation optimizer panics when processing
LogicalPlan::Unnestnodes due to DataFusion's inconsistent behavior withexpressions()vswith_new_exprs().Problem
The federation optimizer would crash with:
plan.with_new_exprs(new_expressions, new_inputs)?because
LogicalPlan::Unnest.expressions()returns expressions butwith_new_exprs()rejects them in the case ofLogicalPlan::Unnest(_).Solution
LogicalPlan::Unnestinoptimize_plan_recursivelyplan.expressions()Testing
test_federation_optimizer_rule_handles_unnestdf-unnest.rsdemonstrating cross-database federation with unnestChecklist
Next Steps
Will also create a follow up PR with DataFusion with a possible solution to the underlying problem. If that gets merged I will come back and clean this up.